[helm-chart] add values for setting annotations and labels for rollout-operator #6733

m1rp · 2023-11-24T14:17:39Z

What this PR does

add labels and annotations used by the rollout-operator.

Which issue(s) this PR fixes or relates to

Fixes #6650
Also very related to This PR over in the rollout-operator repo

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

Extra

i'd love some feedback on this, because not all possible annotation/labels have been added nor are they configurable so some input would be appreciated if this is something worth investing more time into.

CLAassistant · 2023-11-24T14:17:46Z

All committers have signed the CLA.

m1rp · 2023-11-24T14:26:44Z

@56quarters tagging you here as you have some context for these changes, as you're also tagged here and i think it makes sense for these things to be aligned.

56quarters

Thank you for this!

56quarters · 2023-11-24T17:23:37Z

operations/helm/charts/mimir-distributed/templates/alertmanager/alertmanager-statefulset.yaml

@@ -53,11 +53,20 @@ spec:
    metadata:
      labels:
        {{- include "mimir.podLabels" $args | nindent 8 }}
+        grafana.com/no-downscale: {{$rolloutZone.noDownscale}}


I don't think this is needed for the alertmanager, I'm pretty sure they're safe to scale down at any time. Could one of the alerting folks confirm? @stevesg et al

this might also be the case for the store-gateway right?
the resource that needs extra safety when scaling down is mainly the ingester if i'm not mistaken

56quarters · 2023-11-24T17:24:52Z

operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml

@@ -52,8 +52,17 @@ spec:
    metadata:
      labels:
        {{- include "mimir.podLabels" $args | nindent 8 }}
+        grafana.com/no-downscale: {{$rolloutZone.noDownscale}}


These two annotations are mutually exclusive:

A statefulset can either be "no-downscale" in which case, it's not allowed to be scaled down by anything.

Or it can be scaled down by the rollout-operator

does it make sense that the no-downscale is the deciding factor here?
by that i mean that the condition for rendering the prepare-scaledown is that it's value is true and that no-downscaleis false?

@dimitarvdimitrov has suggested that no downscale be the default behavior which slightly simplifies this

sorry, I was interfering. I think having the prepare-scaledown and graceful scaledown procedure by the rollout operator are easier to reason about from an operator's POV - you just scale the StatfulSets as you would normally and the rollout-operator sorts out preparations and wait times.

But I suspect Nick's point was that setting both doesn't make sense. So it would be useful if we add some validations that they both cannot be set. Perhaps you can add a named template (helm function) to this file which checks the values of mimir.podLabels and mimir.podAnnotations and fails the chart rendering when both are set.

oh i see, i didn't know about this possibility!

sorry I didn't make this clear - I think it's better if we have the prepare-downscale and the downscale leader on by default. This would remove some toil from operators as opposed to only preventing accidental downscaling.

56quarters · 2023-11-24T17:26:17Z

operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml

+        grafana.com/prepare-downscale-http-port: 80
+        {{- end-}}
+        {{- if $rolloutZone.downscaleLeader }}
+        grafana.com/rollout-downscale-leader: downscaleLeader


The value of this should be the name of the leader statefulset. For example, the leader of the zone B statefulset should be mimir-ingester-zone-a.

my mistake, i meant to add this as a variable.

56quarters · 2023-11-24T17:26:52Z

operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml

      annotations:
        {{- include "mimir.podAnnotations" $args | nindent 8 }}
+        {{- if $rolloutZone.prepareDownscale }}
+        grafana.com/prepare-downscale-http-path: ingester/prepare-shutdown
+        grafana.com/prepare-downscale-http-port: 80


I don't remember if the default HTTP port for Mimir is changed in the helm chart. This might be 8080 these days.

It's dynamic based on {{ include "mimir.serverHttpListenPort" . }}. It's set in the same file. I think we should use the same template here

dimitarvdimitrov

thanks for the contribution! Can you also add a changelog entry in operations/helm/charts/mimir-distributed/CHANGELOG.md

dimitarvdimitrov · 2023-11-27T19:42:29Z

operations/helm/charts/mimir-distributed/values.yaml

@@ -950,6 +974,14 @@ ingester:
      # If set to "-", then use `storageClassName: ""`, which disables dynamic provisioning
      # If undefined or set to null (the default), then fall back to the value of `ingester.persistentVolume.storageClass`.
      storageClass: null
+      # -- noDownscale adds a label that can be used by the rollout-operator as documented in the rollout-operator repo: https://github.com/grafana/rollout-operator#webhooks
+      noDownscale: false


should we enable this by default in all ingester zones? The idea behind this annotation was to prevent accidental downscaling

operations/helm/charts/mimir-distributed/templates/store-gateway/store-gateway-statefulset.yaml

dimitarvdimitrov · 2023-11-27T19:44:53Z

operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml

      annotations:
        {{- include "mimir.podAnnotations" $args | nindent 8 }}
+        {{- if $rolloutZone.prepareDownscale }}
+        grafana.com/prepare-downscale-http-path: ingester/prepare-shutdown
+        grafana.com/prepare-downscale-http-port: 80


It's dynamic based on {{ include "mimir.serverHttpListenPort" . }}. It's set in the same file. I think we should use the same template here

operations/helm/charts/mimir-distributed/values.yaml

…t-operator Signed-off-by: sam clulow <sam.clulow@springernature.com>

- default `no-downscale` to true - downscale labels are now conditionally rendered - fix path and port

Signed-off-by: sam clulow <sam.clulow@springernature.com>

dimitarvdimitrov · 2024-01-09T11:52:11Z

operations/helm/charts/mimir-distributed/templates/_helpers.tpl

+  "noDownscale":  $rolloutZone.noDownscale
+  "downscaleLeader": $rolloutZone.downscaleLeader
+  "prepareDownscale": $rolloutZone.prepareDownscale


i think the linter is complaining about the colons here

dimitarvdimitrov · 2024-03-25T13:12:42Z

hey, there's something off with the commits in this PR. I see quite a lot of changes and number of commits. Can you take another look please @m1rp?

Signed-off-by: sam clulow <sam.clulow@springernature.com>

m1rp · 2024-03-25T13:32:43Z

apologies, i managed to spectacularly mess up my git history/commit! should be sorted now.

dimitarvdimitrov · 2024-04-08T21:12:36Z

really sorry for the delay 🙈 I was away the week before and swiped away all GH notifications

I can try to take a look later this week after GrafanaCon. @56quarters can you give this one more look?

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

dimitarvdimitrov

dropped a few long-overdue comments. Apologies once again for the delay

dimitarvdimitrov · 2024-05-01T17:13:26Z

operations/helm/charts/mimir-distributed/values.yaml

+        # -- downscaleLeader is an Annotation used by the rollout-operator
+        # If defined, downscaleLeader: <downscaleLeader>
+        # If undefined or set to null (the default), no Annotation is set
+        downscaleLeader: null


would be great if we can set the downscale leader automatically in the zonal templates. That way automatica downscaling can be just a single boolean toggle for the ingester/store-gateway which automatically sets up downscale leader and prepare downscale for the 3 zones.

But i think this PR has been lingering for long enough to try to slow it down further. happy to not include this change in the PR if you want.

having said that - i think having noDownscale: false would be a safer change because it will mean folks can keep running the chart as they used to. If we get automatic downscaling automatically set up like I suggested above, then I think it's viable to have it on by default.

I think it makes sense to try to minimize the added manual effort for folks after they upgrade to a chart version which contains this PR

i think that changing noDownscale: false makes sense. and as you mentioned the PR has been lingering for a while and i just don't have the bandwidth to deal with it.

dimitarvdimitrov · 2024-05-01T17:21:47Z

operations/helm/charts/mimir-distributed/templates/store-gateway/store-gateway-statefulset.yaml

+         {{- if $rolloutZone.prepareDownscale }}
+        grafana.com/prepare-downscale-http-path: store-gateway/prepare-shutdown
+        grafana.com/prepare-downscale-http-port: {{ include "mimir.serverHttpListenPort" . }}
+        {{- end-}}


i think this tag is invalid - it should be {{- end -}} (extra space) It's also invalid for the other two templates

dimitarvdimitrov · 2024-05-01T17:28:55Z

operations/helm/charts/mimir-distributed/templates/store-gateway/store-gateway-statefulset.yaml

+        {{- if (eq $rolloutZone.noDownscale true )}}
+        grafana.com/no-downscale: {{$rolloutZone.noDownscale}}
+        {{- else }}
+        grafana.com/prepare-downscale: {{$rolloutZone.prepareDownscale}}


do we need the annotation even if prepareDownscale is false? I think we can just not apply it

dimitarvdimitrov · 2024-05-01T17:33:52Z

operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml

+        {{- if $rolloutZone.prepareDownscale }}
+        grafana.com/prepare-downscale-http-path: ingester/prepare-shutdown
+        grafana.com/prepare-downscale-http-port: {{ include "mimir.serverHttpListenPort" . }}
+        {{- end-}}
+        {{- if $rolloutZone.downscaleLeader }}
+        grafana.com/rollout-downscale-leader: $rolloutZone.downscaleLeader
+        {{- end }}


we should also set the grafana.com/min-time-between-zones-downscale=12h label on ingesters. store-gateways can work with about 30m (in case lazy loading is disabled). alertmanagers shouldn't need this config

dimitarvdimitrov · 2024-05-01T17:34:27Z

operations/helm/charts/mimir-distributed/templates/store-gateway/store-gateway-statefulset.yaml

    metadata:
      labels:
        {{- include "mimir.podLabels" $args | nindent 8 }}
+        {{- if (eq $rolloutZone.noDownscale true )}}
+        grafana.com/no-downscale: {{$rolloutZone.noDownscale}}
+        {{- else }}
+        grafana.com/prepare-downscale: {{$rolloutZone.prepareDownscale}}
+        {{- end }}
      annotations:
        {{- include "mimir.podAnnotations" $args | nindent 8 }}
+         {{- if $rolloutZone.prepareDownscale }}
+        grafana.com/prepare-downscale-http-path: store-gateway/prepare-shutdown
+        grafana.com/prepare-downscale-http-port: {{ include "mimir.serverHttpListenPort" . }}
+        {{- end-}}
+        {{- if $rolloutZone.downscaleLeader }}
+        grafana.com/rollout-downscale-leader: $rolloutZone.downscaleLeader
+        {{- end }}


these annotations and labels should be set on the stateful set not on the pods themselves.

that's also what jsonnet does

mimir/operations/mimir/ingester-automated-downscale.libsonnet

Lines 34 to 37 in b9d91bd

statefulSet.mixin.metadata.withAnnotationsMixin({

'grafana.com/prepare-downscale-http-path': 'ingester/prepare-shutdown',

'grafana.com/prepare-downscale-http-port': '80',

}),

these are the rollouts operator docs https://github.com/grafana/rollout-operator?tab=readme-ov-file#how-scaling-up-and-down-works

~~is this true of the labels as well?~~ nevermind i looked at the jsonnet and answered my own question i think

dimitarvdimitrov · 2024-05-01T17:35:58Z

i also merged with upstream and did some trivial changes in the changelog and some comments in the values.yaml.

armandgrillet · 2024-12-18T14:40:02Z

@m1rp Thank you for the contribution, can you please do the changes mentioned in the comments? Otherwise, we will close this PR.

chencs · 2024-12-20T23:10:12Z

The CHANGELOG has just been cut to prepare for the next release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the operations/helm/charts/mimir-distributed/CHANGELOG.md document. Thanks!

m1rp force-pushed the rollout-operator-config branch 2 times, most recently from 7f9e087 to 840edc0 Compare November 24, 2023 14:58

56quarters reviewed Nov 24, 2023

View reviewed changes

dimitarvdimitrov reviewed Nov 27, 2023

View reviewed changes

m1rp force-pushed the rollout-operator-config branch from 840edc0 to 446ac64 Compare December 4, 2023 14:33

dimitarvdimitrov reviewed Dec 7, 2023

View reviewed changes

operations/helm/charts/mimir-distributed/values.yaml Outdated Show resolved Hide resolved

m1rp added 3 commits January 2, 2024 14:53

[helm-chart] add values for setting annotations and labels for rollou…

28f8ec9

…t-operator Signed-off-by: sam clulow <sam.clulow@springernature.com>

implement feedback

f4705ac

- default `no-downscale` to true - downscale labels are now conditionally rendered - fix path and port

update changelog

6ff0205

Signed-off-by: sam clulow <sam.clulow@springernature.com>

m1rp force-pushed the rollout-operator-config branch from a844db4 to 6ff0205 Compare January 2, 2024 13:57

m1rp marked this pull request as ready for review January 4, 2024 13:42

m1rp requested a review from a team as a code owner January 4, 2024 13:42

dimitarvdimitrov reviewed Jan 9, 2024

View reviewed changes

m1rp requested review from grafanabot, a team, wilfriedroset, vaxvms, bubu11e and jdbaldry as code owners March 25, 2024 12:46

fix more linting issues

2df4381

Signed-off-by: sam clulow <sam.clulow@springernature.com>

m1rp force-pushed the rollout-operator-config branch from 4b4a6c4 to 2df4381 Compare March 25, 2024 13:14

dimitarvdimitrov mentioned this pull request Mar 27, 2024

Support rollout prepare-downscale in helm chart #7726

Open

dimitarvdimitrov mentioned this pull request Apr 8, 2024

Helm : add hpa for the ingesters #7843

Closed

4 tasks

Merge branch 'main' into rollout-operator-config

ebef306

dimitarvdimitrov added 3 commits May 1, 2024 19:15

Readd deleted comments

7dbd8d2

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

Move changelog entry

de77d1c

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

Fix comment spacing

6a95788

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

dimitarvdimitrov reviewed May 1, 2024

View reviewed changes

implement feedback

81dbb5a

dimitarvdimitrov added the helm label Jun 21, 2024

Merge branch 'main' into rollout-operator-config

ab3a28a

zzehring removed the request for review from grafanabot October 1, 2024 15:10

zerok removed the request for review from a team October 2, 2024 09:45

chencs added the release/notified-changelog-cut label Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[helm-chart] add values for setting annotations and labels for rollout-operator #6733

[helm-chart] add values for setting annotations and labels for rollout-operator #6733

m1rp commented Nov 24, 2023 •

edited

Loading

CLAassistant commented Nov 24, 2023 •

edited

Loading

m1rp commented Nov 24, 2023

56quarters left a comment

56quarters Nov 24, 2023

m1rp Dec 4, 2023

56quarters Nov 24, 2023

m1rp Dec 4, 2023

m1rp Dec 4, 2023

dimitarvdimitrov Dec 7, 2023

m1rp Jan 2, 2024

dimitarvdimitrov Jan 9, 2024

56quarters Nov 24, 2023

m1rp Dec 4, 2023

56quarters Nov 24, 2023

dimitarvdimitrov Nov 27, 2023

dimitarvdimitrov left a comment

dimitarvdimitrov Nov 27, 2023

dimitarvdimitrov Nov 27, 2023

dimitarvdimitrov Jan 9, 2024

dimitarvdimitrov commented Mar 25, 2024

m1rp commented Mar 25, 2024 •

edited

Loading

dimitarvdimitrov commented Apr 8, 2024

dimitarvdimitrov left a comment

dimitarvdimitrov May 1, 2024

dimitarvdimitrov May 1, 2024

m1rp May 28, 2024

dimitarvdimitrov May 1, 2024

dimitarvdimitrov May 1, 2024

dimitarvdimitrov May 1, 2024

dimitarvdimitrov May 1, 2024

m1rp May 28, 2024 •

edited

Loading

dimitarvdimitrov commented May 1, 2024

armandgrillet commented Dec 18, 2024

chencs commented Dec 20, 2024

	statefulSet.mixin.metadata.withAnnotationsMixin({
	'grafana.com/prepare-downscale-http-path': 'ingester/prepare-shutdown',
	'grafana.com/prepare-downscale-http-port': '80',
	}),

[helm-chart] add values for setting annotations and labels for rollout-operator #6733

Are you sure you want to change the base?

[helm-chart] add values for setting annotations and labels for rollout-operator #6733

Conversation

m1rp commented Nov 24, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

Extra

CLAassistant commented Nov 24, 2023 • edited Loading

m1rp commented Nov 24, 2023

56quarters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitarvdimitrov commented Mar 25, 2024

m1rp commented Mar 25, 2024 • edited Loading

dimitarvdimitrov commented Apr 8, 2024

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1rp May 28, 2024 • edited Loading

Choose a reason for hiding this comment

dimitarvdimitrov commented May 1, 2024

armandgrillet commented Dec 18, 2024

chencs commented Dec 20, 2024

m1rp commented Nov 24, 2023 •

edited

Loading

CLAassistant commented Nov 24, 2023 •

edited

Loading

m1rp commented Mar 25, 2024 •

edited

Loading

m1rp May 28, 2024 •

edited

Loading